Proposal: Implementation of Casbin Middleware #676

sugar-cat7 · 2024-07-31T16:35:00Z

Casbin is a library that simplifies authorization control.(ACL, RBAC, etc...)
While major frameworks like Express and Nest have user-defined middleware available, Hono did not have such an implementation. Therefore, I have created one for Hono.

https://casbin.org/docs/middlewares/

changeset-bot · 2024-07-31T16:35:05Z

🦋 Changeset detected

Latest commit: e05e1bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@hono/casbin	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

yusukebe · 2024-08-07T10:23:21Z

Hi @sugar-cat7 !

Sorry for the delayed response. I didin't know the Casbin, but looks good.

I have questions, though I may not understand your intention perfectly. Does the defaultCheckPermission check only the user name of the Basic Authentication header? Does this mean it does not authorize a user with a password from the header?

I wonder if we can use some logic for Basic Auth in hono core. Or is defaultCheckPermission just the default, so we don't have to be so picky?

sugar-cat7 · 2024-08-10T10:34:11Z

@yusukebe
Hello! Thank you for checking this!

First of all, as a premise:
Casbin is more of a framework for authorization rather than authentication. Essentially, it controls "who" can do "what" on "which" resources.
Therefore, it is meant to be used in conjunction with authentication (e.g., something like Basic Auth).
(To give a more concrete example, when using Bearer Auth, you can control whether to allow or deny a user's request by comparing a custom claim, such as "role," contained in the JWT after authentication, with a predefined policy.)

Based on the above:

Does the defaultCheckPermission check only the user name of the Basic Authentication header?

Since this is a framework for authorization, it primarily looks at information about users and their permissions. Regarding defaultCheckPermission, it checks the username and the predefined policy to determine if a user can access a particular endpoint.
(By the way, the reason the default is implemented to look at the Basic Authentication header is to align with the middleware of other frameworks.)

Does this mean it does not authorize a user with a password from the header?

No. Authentication is assumed to be implemented separately, and defaultCheckPermission looks at whether access should be allowed after the user has been authenticated.

I wonder if we can use some logic for Basic Auth in hono core. Or is defaultCheckPermission just the default, so we don't have to be so picky?

It seems that the logic in the following link could be used here:

https://github.com/honojs/hono/blob/8ba02273e829318d7f8797267f52229e531b8bd5/src/middleware/basic-auth/index.ts#L16-L33

However, it would be difficult to use the middleware as-is, so it would require re-implementing the same function (called auth) that retrieves the user.
Currently, exporting the private function implemented on the original Hono side seems a bit awkward for users, right? (It’s not middleware in the first place...)

yusukebe · 2024-08-11T02:55:06Z

Hi @sugar-cat7 Thank you for the response!

I understood the purpose of Casbin well thanks to your explanation.

Therefore, it is meant to be used in conjunction with authentication (e.g., something like Basic Auth).
(To give a more concrete example, when using Bearer Auth, you can control whether to allow or deny a user's request by comparing a custom claim, such as "role," contained in the JWT after authentication, with a predefined policy.)

I think this is ideal and wonderful if we could use this middleware with the current hono's Basic Auth Middleware, Bearer Auth Middleware, or others. Can we do that? If so, I want to see the code base example (it seems you have to change the current code).

Currently, exporting the private function implemented on the original Hono side seems a bit awkward for users, right? (It’s not middleware in the first place...)

Exactly. We don't welcome exporting the private funcitons.

sugar-cat7 · 2024-08-26T10:28:25Z

@yusukebe
Sorry for the delay.

I’ve made some modifications to the codebase.

The changes are as follows:

I separated the main casbin middleware and the authorizer we provide into different directories (src/helper).
I explicitly passed the authorizer to the casbin middleware.
I renamed defaultPermissionCheck to basicAuthorizer.
The basicAuthorizer now uses the same logic as the original Hono implementation. (This is something I’d like to discuss: I’m thinking it might be better to move the auth method to utils. It seems that in other middleware, non-middleware processes are called from utils.)
I also defined jwtAuthorizer in the helper.
Updated the Readme.

I think this is ideal and wonderful if we could use this middleware with the current hono's Basic Auth Middleware, Bearer Auth Middleware, or others. Can we do that? If so, I want to see the code base example (it seems you have to change the current code).

I've included the specific usage for combining them in the README!
Additionally, you might find it easier to understand how to use them together by looking at the test suites for BasicAuthorizer with hono/basic-auth and JWTAuthorizer with hono/jwt.

yusukebe · 2024-09-01T05:45:18Z

Hi @sugar-cat7

Thank you for your explanation!

Creating basicAuthorizer and jwtAuthorizer is good!

The basicAuthorizer now uses the same logic as the original Hono implementation. (This is something I’d like to discuss: I’m thinking it might be better to move the auth method to utils. It seems that in other middleware, non-middleware processes are called from utils.)

As you said, it's better to create auth function in utils. I'm not 100% sure, but I also faced the same issue when I wanted to implement Basic Auth.

The type of auth will be good with the following:

type Auth = (request: Request) => { username: string; password: string }

I can do it later, but can you create a PR to refactor like that on honojs/hono?

sugar-cat7 · 2024-09-02T08:47:34Z

@yusukebe

I can do it later, but can you create a PR to refactor like that on honojs/hono?

I have created a PR to move the functions to the utils directory!
honojs/hono#3359

sugar-cat7 · 2024-09-03T09:16:27Z

@yusukebe
I have incorporated the content from hono v4.5.11 and removed any duplicate sections.

yusukebe · 2024-09-03T10:41:21Z

package.json

Change as following to export modules correctly:

diff --git a/packages/casbin/package.json b/packages/casbin/package.json index 6d0be5f..da02259 100644 --- a/packages/casbin/package.json +++ b/packages/casbin/package.json @@ -3,9 +3,9 @@ "version": "1.0.1", "description": "Casbin middleware for Hono", "type": "module", - "main": "dist/cjs/index.js", - "module": "dist/esm/index.js", - "types": "dist/esm/index.d.ts", + "main": "dist/index.cjs", + "module": "dist/index.js", + "types": "dist/index.d.ts", "exports": { ".": { "import": { @@ -16,6 +16,16 @@ "types": "./dist/index.d.cts", "default": "./dist/index.cjs" } + }, + "./helper": { + "import": { + "types": "./dist/helper/index.d.ts", + "default": "./dist/helper/index.js" + }, + "require": { + "types": "./dist/helper/index.d.cts", + "default": "./dist/helper/index.cjs" + } } }, "files": [ @@ -23,7 +33,7 @@ ], "scripts": { "test": "vitest --run", - "build": "tsup ./src/index.ts --format esm,cjs --dts", + "build": "tsup ./src/index.ts ./src/helper/index.ts --format esm,cjs --dts", "publint": "publint", "release": "yarn build && yarn test && yarn publint && yarn publish" }, @@ -48,4 +58,4 @@ "typescript": "^5.5.3", "vitest": "^2.0.1" } -} +}

Fixed!
18b9d2a

yusukebe · 2024-09-03T10:41:51Z

packages/casbin/README.md

+import { Hono } from 'hono'
+import { basicAuth } from 'hono/basic-auth'
+import { newEnforcer } from 'casbin'
+import { casbin } from '@hono/cabin'


It's a typo. Should be @hono/casbin'.

Fixed!
d0325f1

yusukebe · 2024-09-03T10:42:09Z

packages/casbin/README.md

+import { basicAuth } from 'hono/basic-auth'
+import { newEnforcer } from 'casbin'
+import { casbin } from '@hono/cabin'
+import { basicAuthorizer } from '@hono/cabin/helper'


Also a typo.

Fixed!
d0325f1

packages/casbin/README.md

yusukebe · 2024-09-03T10:57:42Z

@sugar-cat7

Thanks! I've added some comments.

yusukebe · 2024-09-03T10:58:12Z

Sorry, I accidentally closed it.

sugar-cat7 · 2024-09-08T15:22:08Z

@yusukebe
Thank you for the review!
I've made the corrections!

yusukebe

LGTM!

packages/casbin/package.json

yusukebe · 2024-09-09T12:56:08Z

@sugar-cat7

Thank! Let's go!

sugar-cat7 · 2024-09-09T13:09:43Z

@yusukebe
I'm sorry, it seems like the method I used to resolve the yarn.lock conflict wasn't good, and it's causing the CI to fail. Should I revert it?

yusukebe · 2024-09-09T13:10:44Z

@sugar-cat7 No worry. I'm fixing it.

yusukebe · 2024-09-09T13:13:40Z

Fixed!

exoego · 2024-09-09T14:12:23Z

@sugar-cat7
I created casbin/casbin-website-v2#264 for Hono users 😉

feat: impl casbin middleware

8a4588e

Merge branch 'main' into feat/casbin-middleware

69e051e

sugar-cat7 marked this pull request as ready for review July 31, 2024 16:45

sugar-cat7 changed the title ~~Proposal: Implementation of Casbin Third-Party Middleware~~ Proposal: Implementation of Casbin Middleware Jul 31, 2024

sugar-cat7 added 3 commits August 26, 2024 14:30

fix: defaultCheckPermission logic

c82519b

fix: divide authorizer

7085c9d

chore: testing and docs

affbb67

sugar-cat7 mentioned this pull request Sep 2, 2024

refactor(utils/basic-auth): Moved Internal function to utils honojs/hono#3359

Merged

4 tasks

fixed: hono version

e0247df

sugar-cat7 force-pushed the feat/casbin-middleware branch from c6cd225 to e0247df Compare September 3, 2024 09:13

yusukebe reviewed Sep 3, 2024

View reviewed changes

packages/casbin/README.md Show resolved Hide resolved

yusukebe closed this Sep 3, 2024

yusukebe reopened this Sep 3, 2024

sugar-cat7 added 5 commits September 8, 2024 23:53

fix: export and module resolution

18b9d2a

fix: typo

d0325f1

docs: fix model

4749e9d

Merge branch 'main' into feat/casbin-middleware

47a44fb

fix: conflict

261b6c9

sugar-cat7 requested a review from yusukebe September 8, 2024 15:21

yusukebe approved these changes Sep 9, 2024

View reviewed changes

yusukebe reviewed Sep 9, 2024

View reviewed changes

packages/casbin/package.json Outdated Show resolved Hide resolved

fix: version

e05e1bb

yusukebe merged commit b7e740f into honojs:main Sep 9, 2024

github-actions bot mentioned this pull request Sep 9, 2024

Version Packages #736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Implementation of Casbin Middleware #676

Proposal: Implementation of Casbin Middleware #676

sugar-cat7 commented Jul 31, 2024

changeset-bot bot commented Jul 31, 2024 •

edited

Loading

yusukebe commented Aug 7, 2024

sugar-cat7 commented Aug 10, 2024

yusukebe commented Aug 11, 2024

sugar-cat7 commented Aug 26, 2024 •

edited

Loading

yusukebe commented Sep 1, 2024

sugar-cat7 commented Sep 2, 2024

sugar-cat7 commented Sep 3, 2024

yusukebe Sep 3, 2024

sugar-cat7 Sep 8, 2024

yusukebe Sep 3, 2024

sugar-cat7 Sep 8, 2024

yusukebe Sep 3, 2024

sugar-cat7 Sep 8, 2024

yusukebe commented Sep 3, 2024

yusukebe commented Sep 3, 2024

sugar-cat7 commented Sep 8, 2024

yusukebe left a comment

yusukebe commented Sep 9, 2024

sugar-cat7 commented Sep 9, 2024

yusukebe commented Sep 9, 2024

yusukebe commented Sep 9, 2024

exoego commented Sep 9, 2024

Proposal: Implementation of Casbin Middleware #676

Proposal: Implementation of Casbin Middleware #676

Conversation

sugar-cat7 commented Jul 31, 2024

changeset-bot bot commented Jul 31, 2024 • edited Loading

🦋 Changeset detected

yusukebe commented Aug 7, 2024

sugar-cat7 commented Aug 10, 2024

yusukebe commented Aug 11, 2024

sugar-cat7 commented Aug 26, 2024 • edited Loading

yusukebe commented Sep 1, 2024

sugar-cat7 commented Sep 2, 2024

sugar-cat7 commented Sep 3, 2024

yusukebe Sep 3, 2024

Choose a reason for hiding this comment

sugar-cat7 Sep 8, 2024

Choose a reason for hiding this comment

yusukebe Sep 3, 2024

Choose a reason for hiding this comment

sugar-cat7 Sep 8, 2024

Choose a reason for hiding this comment

yusukebe Sep 3, 2024

Choose a reason for hiding this comment

sugar-cat7 Sep 8, 2024

Choose a reason for hiding this comment

yusukebe commented Sep 3, 2024

yusukebe commented Sep 3, 2024

sugar-cat7 commented Sep 8, 2024

yusukebe left a comment

Choose a reason for hiding this comment

yusukebe commented Sep 9, 2024

sugar-cat7 commented Sep 9, 2024

yusukebe commented Sep 9, 2024

yusukebe commented Sep 9, 2024

exoego commented Sep 9, 2024

changeset-bot bot commented Jul 31, 2024 •

edited

Loading

sugar-cat7 commented Aug 26, 2024 •

edited

Loading